BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

mingweiarthurli · 2024-02-16T02:51:53Z

Please merge this PR before merging eclipse-openj9/openj9#17813!

This is a pre-requisite PR of eclipse-openj9/openj9#17813, which used _hasBackEdges flag to determine if the generated CFG has back edges, and then used the generated CFG to generate the dominators and perform structural analysis. The structural analysis of using the given CFG enables the BenefitInliner to performance abstract interpretation for loop.

mingweiarthurli · 2024-02-16T20:34:31Z

Hi @jdmpapin, could you please take a look for this PR? Thank you! This is a pre-requisition by eclipse-openj9/openj9#17813, which are some code for handling loop by the BenefitInliner.

These changes were commited in eclipse-openj9/openj9#17813, but I treated them as obsolete code by mistake and discarded them. I'm sorry for the confusion and thank you very much for taking time to review this extra PR.

Since you had some questions for the changes in last PR, I will answer all of them at once now.

BenefitInliner phase 3/3: Classes for doing nested knapsack algorithm and the inlining. #5509 (comment) and BenefitInliner phase 3/3: Classes for doing nested knapsack algorithm and the inlining. #5509 (comment):
The CFG used by the newly added constructor TR_Dominators::TR_Dominators(TR::Compilation *c, TR::CFG* cfg, bool post) , rather than is from IDTBuilder::generateControlFlowGraph(), actually is from node->getCallTarget()->_cfg, which is from a TR_CallTarget. I run the testcase of Jython and set breakpoint after if (_topDfNum != _numNodes-1) in our constructor, and I do found sometimes the breakpoint will be triggered. It seems the CFG gotten from the IDTNode's call target sometimes contains the unreachable blocks, and we don't want them, so the newly added TR_RegionAnalysis::getRegions() will do an early return once the dominator constructor found there is any unreachable blocks.
The problem is: TR_ASSERT seems just mapped to void(0) and will do nothing, so the former contributor chose to use _isValid to represent if the dominator detected any unreachable blocks.
I refactored this part and changed to use _hasUnreachableBlocks to represent existing unreachable block for code clarity.
BenefitInliner phase 3/3: Classes for doing nested knapsack algorithm and the inlining. #5509 (comment):
I also refactored this part.

Thank you for your time in advance!

mingweiarthurli · 2024-02-16T21:26:39Z

Currently the macOS check is failed, and the terminal ouput are following:

2: Traceback (most recent call last):
2:   File "/Users/runner/work/1/s/jitbuilder/apigen/test/cppgentests.py", line 128, in test_to_opaque_cast_2
2:     self.assertRegexpMatches(self.generator.to_opaque_cast("bar", class_desc),
2:     ^^^^^^^^^^^^^^^^^^^^^^^^
2: AttributeError: 'CppGeneratorTest' object has no attribute 'assertRegexpMatches'
2: 
2: ----------------------------------------------------------------------
2: Ran 143 tests in 0.845s
2: 
2: FAILED (errors=25, skipped=3)
2: warning: The package 'jsonschema' is not installed so certain tests will be skipped
32/49 Test  #2: TestJitBuilderAPIGenerator ..........***Failed    1.01 sec

This is still a known issue of #7181.

jdmpapin

Thanks for the refactoring here!

First I want to mention that there's a merge conflict. Could you please rebase to fix the conflict and push before making any other changes? Rebasing will introduce many changes compared to the current revision of the branch, and doing it this way will keep those separate from the meaningful changes that follow

I run the testcase of Jython and set breakpoint after if (_topDfNum != _numNodes-1) in our constructor, and I do found sometimes the breakpoint will be triggered. It seems the CFG gotten from the IDTNode's call target sometimes contains the unreachable blocks, and we don't want them, so the newly added TR_RegionAnalysis::getRegions() will do an early return once the dominator constructor found there is any unreachable blocks.

The problem is: TR_ASSERT seems just mapped to void(0) and will do nothing, so the former contributor chose to use _isValid to represent if the dominator detected any unreachable blocks.
I refactored this part and changed to use _hasUnreachableBlocks to represent existing unreachable block for code clarity.

OK. I think the real problem is that there are unreachable blocks in the CFG. They shouldn't be generated, or they should be removed before we get to this point. The TR_ASSERT is only checked in debug builds, but it's clearly stating that unreachable blocks are expected to be impossible here. I think we should probably change it to TR_ASSERT_FATAL so that it will fail in production builds as well

However, I don't want to delay this PR any further to deal with the fallout of that kind of change. So I'm OK with the general strategy already implemented here. The relaxation of the assertion is limited to the case where a CFG is passed explicitly, which will only be in the benefit inliner. But I have some comments about the details

jdmpapin · 2024-06-11T16:01:08Z

compiler/infra/OMRCfg.hpp

@@ -357,6 +358,9 @@ class CFG
      IsOrphanedRegion
      };

+   void setHasBackEdges() { _hasBackEdges = true; }
+   bool hasBackEdges() { return _hasBackEdges; }


This state isn't meaningfully set or used in this PR - I assume it's used in eclipse-openj9/openj9#17813

Does this need to be on the CFG? Or could it be kept somewhere else instead? e.g. next to a CFG pointer in the code that cares about it. I'm just worried that it will easily get out of date. In fact, with the fixed initial value (false), it will immediately be out of date for many CFGs. (And that would still be the case if the fixed initial value were true instead.)

jdmpapin · 2024-06-11T16:30:59Z

compiler/optimizer/Dominators.cpp

@@ -110,15 +124,22 @@ TR_Dominators::TR_Dominators(TR::Compilation *c, bool post) :
            traceMsg(comp(), "Some blocks are not reachable from exit. Post-dominator info is invalid.\n");
         return;
         }
+      if (!acceptUnreachableBlocks)


I think that the name acceptUnreachableBlocks is misleading - it's almost exactly backwards. The assertion isn't there to accept/ignore unreachable blocks. Rather, it's there to crash if it finds one

It might be better to have the name describe the input, e.g. what do you think of cfgMayContainUnreachableBlocks? If that's true, then we have to deal with the unreachable blocks (by indicating that they were found), and otherwise we can fail the assertion because they aren't supposed to be there

jdmpapin · 2024-06-11T18:02:41Z

compiler/optimizer/Dominators.cpp

@@ -110,15 +124,22 @@ TR_Dominators::TR_Dominators(TR::Compilation *c, bool post) :
            traceMsg(comp(), "Some blocks are not reachable from exit. Post-dominator info is invalid.\n");
         return;
         }
+      if (!acceptUnreachableBlocks)
+         {
+         _hasUnreachableBlocks = true;


Please stick with _isValid, which already has the same meaning, and remove _hasUnreachableBlocks

jdmpapin · 2024-06-11T18:06:47Z

compiler/optimizer/Dominators.hpp

@@ -53,11 +53,14 @@ class TR_Dominators
   TR_ALLOC(TR_Memory::Dominators)

   TR_Dominators(TR::Compilation *, bool post = false);
+   TR_Dominators(TR::Compilation *, TR::CFG* cfg, bool acceptUnreachableBlocks = true, bool post = false);


Does acceptUnreachableBlocks need to be a parameter? I expect TR_RegionAnalysis::getRegions(...cfg...) to be the only use of this. It's a parameter there as well, but I think it probably doesn't need to be

jdmpapin · 2024-06-11T18:24:52Z

compiler/optimizer/Dominators.hpp

   TR::Block       *getDominator(TR::Block *);
   int             dominates(TR::Block *block, TR::Block *other);

   TR::Compilation * comp()         { return _compilation; }
   bool trace() { return _trace; }
+   bool isValid() { return _isValid; }


Could you please add a comment to isValid() that explains why it's now possible for this to be invalid? e.g. something like this:

// HACK: Dominators should always be valid, but for some reason the benefit // inliner can pass a CFG with unreachable blocks.

jdmpapin · 2024-06-11T18:33:13Z

compiler/optimizer/Dominators.cpp

+         {
+         _hasUnreachableBlocks = true;
+         if (trace())
+            traceMsg(comp(), "Unreachable block in the CFG %d %d\n", _topDfNum, _numNodes-1);


Can this message mention that the dominators are therefore invalid, like in the postdominator case?

jdmpapin · 2024-06-11T18:33:59Z

compiler/optimizer/Dominators.cpp

+         if (trace())
+            traceMsg(comp(), "Unreachable block in the CFG %d %d\n", _topDfNum, _numNodes-1);
+         return;
+         }
      else
         TR_ASSERT(false, "Unreachable block in the CFG %d %d", _topDfNum, _numNodes-1);


Would you mind adding a TODO comment for changing this to TR_ASSERT_FATAL?

… when generating CFG This is a part of the BenefitInliner contribution. * Dominators.hpp, Dominators.cpp, StructuralAnalysis.hpp, StructuralAnalysis.cpp: add structural analysis by using given CFG. * OMRCfg.hpp: add _hasBackEdges flag to mark having loop in the CFG. Co-authored-by: Cijie Xia <[email protected]> Signed-off-by: Mingwei Li <[email protected]>

mingweiarthurli · 2024-07-15T19:05:42Z

Hi Devin, just would like to tell you that 2b08c10 is just a rebase of preexisting code. The previous conflict was caused by newly added code from OMR master branch at line 362 and newly added code from this PR at line 364-365 of OMRCfg.hpp. I kept both of them as a fix. Currently there wasn't any meaningful changes yet. I will submit them in my next force-pushed commit.

mingweiarthurli requested review from vijaysun-omr, 0xdaryl and mstoodle as code owners February 16, 2024 02:51

mingweiarthurli mentioned this pull request Feb 16, 2024

Add classes for building IDT and performing abstract interpretation. eclipse-openj9/openj9#17813

Open

github-actions bot added the comp:compiler label Feb 16, 2024

mingweiarthurli changed the title ~~BenefitInliner: Add structual analysis by using given CFG and add _hasBackEdges flag when generating CFG~~ BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG Feb 16, 2024

mingweiarthurli force-pushed the jack-inliner-loop-new branch from d2d79a1 to bf109fc Compare February 16, 2024 19:19

mingweiarthurli force-pushed the jack-inliner-loop-new branch from bf109fc to c2939f2 Compare February 16, 2024 20:35

jdmpapin reviewed Jun 11, 2024

View reviewed changes

mingweiarthurli force-pushed the jack-inliner-loop-new branch from c2939f2 to 2b08c10 Compare July 15, 2024 00:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Feb 16, 2024

jdmpapin left a comment

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

jdmpapin Jun 11, 2024

mingweiarthurli commented Jul 15, 2024 •

edited

Loading

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

Are you sure you want to change the base?

BenefitInliner: Add structural analysis by using given CFG and add _hasBackEdges flag when generating CFG #7268

Conversation

mingweiarthurli commented Feb 16, 2024 • edited Loading

mingweiarthurli commented Feb 16, 2024 • edited Loading

mingweiarthurli commented Feb 16, 2024

jdmpapin left a comment

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

jdmpapin Jun 11, 2024

Choose a reason for hiding this comment

mingweiarthurli commented Jul 15, 2024 • edited Loading

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Feb 16, 2024 •

edited

Loading

mingweiarthurli commented Jul 15, 2024 •

edited

Loading